Skip to content

Feature template picker #993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6,129 commits into
base: v.next
Choose a base branch
from
Open

Feature template picker #993

wants to merge 6,129 commits into from

Conversation

rolson
Copy link
Contributor

@rolson rolson commented Dec 28, 2024

We have one of these in the objc toolkit.

This is an initial PR. I still need to add tests and a tutorial. Also the map that I use in the example is not a good map for this. I need to find a better map.

image

dfeinzimer and others added 30 commits October 24, 2024 13:15
Re-enable `FeatureFormViewTests/testCase_7_1`
Re-enable `FeatureFormViewTests/testCase_3_6`
`FeatureFormView` / `ComboBoxInput` - Show unsupported values
`FeatureFormView`/`InputFooter` - Handle all possible numeric domain field types
Support `TextFormElement`s inside a `GroupFormElement`
`FeatureFormView` - 200.6 doc updates
[Update] Table top `translationFactor` doc
@mhdostal
Copy link
Member

mhdostal commented Jan 3, 2025

Ok, I fixed the image size so that it will limit it to 50x50, but not enlarge the smaller icon sizes.

The only issue I see with that is when Dynamic Type is used the text will increase in size but not the icons. That would be "nice-to-have", but it's not a dealbreaker for me.

image

I did add padding around the image to keep the size in check.

Image(uiImage: image)
    .resizable()
    .aspectRatio(contentMode: .fit)
    .padding(4)

Here's what it looks like with Dynamic Type:

image

And without it:

image

@rolson
Copy link
Contributor Author

rolson commented Jan 3, 2025

The only issue I see with that is when Dynamic Type is used the text will increase in size but not the icons. That would be "nice-to-have", but it's not a dealbreaker for me.

I'd rather have features that are under 50x50 draw correctly in their correct size. To me that's important. If there is a way to do that and let the larger features grow up to the dynamic font size, that would be ideal. But I couldn't find a way to do that unfortunately.

@philium
Copy link
Contributor

philium commented Jan 3, 2025

The tool for that job is ScaledMetric.

@rolson rolson force-pushed the ryan/featureTemplatePicker branch from 32af9db to 3ffb37d Compare January 3, 2025 20:46
@rolson rolson requested review from mhdostal and dfeinzimer January 3, 2025 21:58
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 2 last comments and I think that'll be it.

Comment on lines +262 to +263
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the templates be sorted alphabetically by name?

Suggested change
)
}
)
infos.sort { $0.template.name < $1.template.name }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the sections too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I could see alphabetizing them. Currently it is in the order of layers and then the templates that the layers report. So I'm not sure what is desirable. @mhdostal do you recall what the obj-c based version did?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I could see alphabetizing them. Currently it is in the order of layers and then the templates that the layers report. So I'm not sure what is desirable. @mhdostal do you recall what the obj-c based version did?

There was no sorting in the obj-c template picker.

@mhdostal
Copy link
Member

mhdostal commented Jan 6, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

mhdostal
mhdostal previously approved these changes Jan 6, 2025
@rolson
Copy link
Contributor Author

rolson commented Jan 7, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

Cool. Did the original template picker in the other toolkit do this as well?

@mhdostal
Copy link
Member

mhdostal commented Jan 7, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

Cool. Did the original template picker in the other toolkit do this as well?

No, but it's something that I'm doing for the Swift Feature Form test app; we're not concerned with the full geometry-editing experience yet, so we're only supporting adding point features.

dfeinzimer
dfeinzimer previously approved these changes Jan 7, 2025
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the image. PR looks good 👍

mhdostal
mhdostal previously approved these changes Jan 15, 2025
dfeinzimer
dfeinzimer previously approved these changes Mar 10, 2025
@mhdostal mhdostal dismissed stale reviews from dfeinzimer and themself via 02ebdcf May 22, 2025 21:49
@mhdostal mhdostal force-pushed the ryan/featureTemplatePicker branch from 5f2f894 to 02ebdcf Compare May 22, 2025 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants